Hey @GvMariani ,

First of all: thanks for your PR!

So, LADISH uses a different version of the lash.h, right? Is it shipped with the package? In the Debian package there is no such header present.

That said LADISH doesn't seem to be a lot more lively than LASH itself. Last bigger touches of the code were done almost exactly 10 years ago and it seems to be dropped by Debian after buster.

Don't require session handling library to be used.

Huh? Isn't LADISH the session handling library we want to use in this PR? Or is it referring to other libraries?

Allow clients to use external storage to save their state

For this to work properly a number of changes have to be made in the code. But such things are already implemented as part of the NSM support.

While transitioning to NSM it could be useful do this change.

Hydrogen already is fully NSM compatible.

So, LADISH uses a different version of the lash.h, right? Is it shipped with the package? In the Debian package there is no such header present.

No. If you build ladish and enable the optional lash compatibility (by passing -Dliblash=enabled to meson) you get the compat headers in $ijncludedir/lash-1.0 and the corresponding pkgconfig file; the header exports exactly the same API as the original lash, thus allowing the simple switch I made in this PR, apparently without any issue.

That said LADISH doesn't seem to be a lot more lively than LASH itself. Last bigger touches of the code were done almost exactly 10 years ago and it seems to be dropped by Debian after buster.

Well, it's used in active projects, so if need arises I guess the code base will get updates, while with the original lash this is not happening at all...

Don't require session handling library to be used.
Huh? Isn't LADISH the session handling library we want to use in this PR? Or is it referring to other libraries?

I quoted the readme file from the ladish as-is; it then explains: "There is no need of such library for restoring connections between JACK clients." I missed the last sentence in the cut-and-paste...

Allow clients to use external storage to save their state
For this to work properly a number of changes have to be made in the code. But such things are already implemented as part of the NSM support.

So non need to add it by using lash/ladish: with the change proposed here you should have exactly the same feature support level you have now.

While transitioning to NSM it could be useful do this change.
Hydrogen already is fully NSM compatible.

My bad: wrong wording...
I would have said: "while jack is transitioning": if I'm not mistaken the jack devels are going to deprecate and remove support for other means to manage sessions and go with NSM... Until this process is done, IMO there is still space for this little change; when it will be done, then all the lash/ladish stuff can be dropped. E.g. in the distribution I'm using (ROSA Linux) NSM is not yet packaged, so to have jack session management I have to rely on lash or ladish...
However, if the change is deemed useless this PR can simply closed.

No. If you build ladish and enable the optional lash compatibility (by passing -Dliblash=enabled to meson) you get the compat headers in $ijncludedir/lash-1.0 and the corresponding pkgconfig file; the header exports exactly the same API as the original lash, thus allowing the simple switch I made in this PR, apparently without any issue.

The reason the automated tests did not fail on this PR is because LASH support is disabled per-default (and none on of the LASH features are covered by unit tests). You have to add the -DWANT_LASH=On option to cmake to enable it. Be sure to check the output of cmake as the LASH support will just be omitted in case no lash.h header was found (instead of throwing an error).

Compiling Hydrogen with LASH support can be done by the user herself or by the package maintainers of the individual Linux distros (as we do not ship Linux binaries or packages). But as neither LASH nor LADISH is present in Debian, at least in this OS Hydrogen wouldn't be available with LASH supported via the default repos.

But I'm also still a little bit puzzled regarding the objective of your PR. As the lash.h header stays shipped with LADISH is identical to the one found in LASH packages itself, the PR is about being able to compile Hydrogen with LASH support in case only LADISH and not LASH itself is already installed.

Well, it's used in active projects, so if need arises I guess the code base will get updates

Do you know whether the maintainer announced to maintain the LASH protocol as well? Or are these updates only confined to the LADISH code itself?

with the change proposed here you should have exactly the same feature support level you have now.

Well, in order to support the external storage quite a number of changes have to be made to the code base.

if I'm not mistaken the jack devels are going to deprecate and remove support for other means to manage sessions and go with NSM

Ah, I see. But AFAIK only JACK Session is affected and not LASH. But I'm not sure and have to check it

Hi there. This might be a naïve question, but anyway.

What about allowing to build with lash or with ladish?

Hence the changes would look something like:

FIND_HELPER(LASH lash-1.0 lash/lash.h lash)
+FIND_HELPER(LASH liblash lash-1.0/lash/lash.h lash)

instead of

-FIND_HELPER(LASH lash-1.0 lash/lash.h lash)
+FIND_HELPER(LASH liblash lash-1.0/lash/lash.h lash)

this would leave the possibility for the users/packagers to build with whatever is available on his system.

The reason the automated tests did not fail on this PR is because LASH support is disabled per-default (and none on of the LASH features are covered by unit tests). You have to add the -DWANT_LASH=On option to cmake to enable it. Be sure to check the output of cmake as the LASH support will just be omitted in case no lash.h header was found (instead of throwing an error).
Compiling Hydrogen with LASH support can be done by the user herself or by the package maintainers of the individual Linux distros (as we do not ship Linux binaries or packages).

I'm building (locally and on the ROSA build servers: see with -DWANT_LASH=On and got no failures at all.

But as neither LASH nor LADISH is present in Debian, at least in this OS Hydrogen wouldn't be available with LASH supported via the default repos.

Oh I did not checked that...
Yes, there is ladish, but only in stretch and buster. And everyone else only have only lash...
I guess this means that my PR does not make sense, because nobody could use ladish... I would then retire the PR.
But the following suggestion perhaps is a better way to go...

What about allowing to build with lash or with ladish? Hence the changes would look something like:
FIND_HELPER(LASH lash-1.0 lash/lash.h lash)
+FIND_HELPER(LASH liblash lash-1.0/lash/lash.h lash)
instead of
-FIND_HELPER(LASH lash-1.0 lash/lash.h lash)
+FIND_HELPER(LASH liblash lash-1.0/lash/lash.h lash)

this would leave the possibility for the users/packagers to build with whatever is available on his system.

This would be better than my proposal...

Okay, we might talk about different things here. Is the PR about using a) LADISH incl. its full feature set instead of LASH or b) allowing to build Hydrogen with LASH support using the lash.h header provided by LADISH? For the full feature set a number of changes in the code are required. Session folders are not supported right away and recovering JACK connections won't probably work either and requires the same workaround as used for NSM.

I'm building (locally and on the ROSA build servers: see with -DWANT_LASH=On and got no failures at all.

Nice.

I'm a little bit surprised about it since you didn't put # in front of the include statements and the code wouldn't compile on my machine.

I guess this means that my PR does not make sense, because nobody could use ladish... I would then retire the PR.

Well, if it is present in ROSA Linux than this alone would be enough reason to keep it.

Do you by any chance know the maintainer of Hydrogen or LADISH for ROSA Linux? If they intend to keep LADISH for a longer period of time, this PR makes perfect sense. But if they are about to drop it too, that's a different story.

I just searched around a bit and unfortunately almost all resources of LADISH are down by now. Not even the docs can be accessed anymore (due to which it does not feel like a maintained thing at all). Therefore I don't really know how it works or what kind of changes we would have to make in order to fully support it.

Hello, LADISH author/maintainer here,

liblash implementation that is part of LADISH codebase is for providing drop-in replacement for lash-aware apps.
In theory and IIRC, the header inclusion tweak that I see is being used is not what I intended when liblash for ladish was created. That is, lash header include lines should stay same no matter if it is lash or ladish. The different include path may be related to the meson build system that was merged recently.

I would like to emphasize that LADISH is a multi-protocol session manager. It can handle apps via multiple session management protocols along with apps that support none.

NSM support in ladish was planned ten years ago when the OSC NSM 1.0 protocol was designed. Supporting apps implementing New-Session Manager protocol (1.1.1 now) is also currently planned.

NSM protocols offer some additional features: the optional-gui - the ability to show/hide UIs, and the ability to switch app state without relaunching the app. The former feature is likely to get into LADISH sooner than the other.

New Session Manager 1.1.X OSC API changes the optional-gui behaviour by adding this to documentation "The client SHOULD always start hidden, if not saved as visible. That implies the first load, after adding to the session, SHOULD always be hidden.". So for now I treat them in LADISH as two slightly different protocols to support.
I.e. https://ladish.org/nsm-proto.html vs https://new-session-manager.jackaudio.org/api/index.html

Okay, we might talk about different things here. Is the PR about using a) LADISH incl. its full feature set instead of LASH or b) allowing to build Hydrogen with LASH support using the lash.h header provided by LADISH? For the full feature set a number of changes in the code are required. Session folders are not supported right away and recovering JACK connections won't probably work either and requires the same workaround as used for NSM.

It's b)...

I'm building (locally and on the ROSA build servers: see with -DWANT_LASH=On and got no failures at all.
Nice.
I'm a little bit surprised about it since you didn't put # in front of the include statements and the code wouldn't compile on my machine.

Oops... yeah: # is needed. I did not notice that I was testing my original patch for ROSA, not the obviously wrong one proposed here... sorry.

I guess this means that my PR does not make sense, because nobody could use ladish... I would then retire the PR.
Well, if it is present in ROSA Linux than this alone would be enough reason to keep it.
Do you by any chance know the maintainer of Hydrogen or LADISH for ROSA Linux? If they intend to keep LADISH for a longer period of time, this PR makes perfect sense. But if they are about to drop it too, that's a different story.

Yes, I'm working as de facto maintainer of Sound packages in ROSA Linux, including Hydrogen...
About lash/ladish: ATM we have both in our repositories, but I'm pushing to drop lash (surely unmaintained) and use ladish (as replacement (where applicable), to avoid feature loss in our packages (usually we enable lash use in them...).
In the mean time I will try to package the NSM (ATM we don't have it) and when it will become mandatory for jack, I will push to use it instead of ladish...

I just searched around a bit and unfortunately almost all resources of LADISH are down by now. Not even the docs can be accessed anymore (due to which it does not feel like a maintained thing at all). Therefore I don't really know how it works or what kind of changes we would have to make in order to fully support it.

Do you mean "docs other than the ones present in the git repositories"? I don't know, but perhaps you could get in touch with the author (he commented above in this PR...).

@GvMariani LADISH aims to support apps via NSM as well. So at some point in future you could have option to support NSM via LADISH (inclusive)or some other session manager (e.g. RaySession) that already exist for the NSM protocols 1.0 and 1.1.X.

A full set of LADISH features that H2 can additionally implement is probably empty. While being reimplementation of LASH (strictly speaking a fork of late lash+dbus development), LADISH implementation did not extend the LASH API (protocol).
The only LADISH specific protocol is the so called L1, which is just saving when SIGUSR1 is received by the app.
When apps to be started under ladish session management are added, the communication protocol (API) to use is specified. With options being NONE (just connections are managed/saved/restored and not app state), L1 (SIGUSR1), LASH and JackSession. In future NSM is going to be provided via a new software component Jack Plugin Launcher.

I.e. a) is probably not existing option at all.

New Session Manager 1.1.X OSC API changes the optional-gui behaviour by adding this to documentation "The client SHOULD always start hidden, if not saved as visible. That implies the first load, after adding to the session, SHOULD always be hidden.". So for now I treat them in LADISH as two slightly different protocols to support.

Yikes! Sounds like someone baked his/her preferred workflow routine into the forked protocol.

It's b)

Glad to hear! The title of the PR smelled like a lot of changes in the code base. But header lookup is fine.

About lash/ladish: ATM we have both in our repositories, but I'm pushing to drop lash (surely unmaintained) and use ladish (as replacement (where applicable), to avoid feature loss in our packages (usually we enable lash use in them...).
In the mean time I will try to package the NSM (ATM we don't have it) and when it will become mandatory for jack, I will push to use it instead of ladish...

If you want to ship Hydrogen with LASH supported, we'll of course do our best to make this happen. Preferably by adding a fallback lookup for the headers as @trebmuh suggested.

Regarding your future plans: After checking out LASH and having seen how few working references are left in the web I might propose to drop Hydrogen's support for it too. Not in the upcoming release (1.2), not in 1.3 (where it would be marked deprecated), but in 1.4. So, that's at least two more years till it gets dropped.

LADISH aims to support apps via NSM as well

Great! NSM support in Hydrogen is well tested.

The only LADISH specific protocol is the so called L1, which is just saving when SIGUSR1 is received by the app.

AFAICS we already support this.